Splitattrs ncsave redo commonmeta #5538
Conversation
|
Ping @trexfeathers This should be the last piece of the puzzle which completes https://github.com/orgs/SciTools/projects/5 |
| "same": ("GaLb:GaLb", [True]), | ||
| "extra_global": ("GaL-:G-L-", [False, True]), | ||
| "extra_local": ("G-La:G-L-", [False, True]), | ||
| "same_global_local": ("GaL-:G-La", [False, True]), | ||
| "diff_global_local": ("GaL-:G-Lb", [False, True]), | ||
| "diffglobal_nolocal": ("GaL-:GbL-", [False]), | ||
| "diffglobal_samelocal": ("GaLc:GbLc", [False]), | ||
| "difflocal_noglobal": ("G-La:G-Lb", [False]), | ||
| "difflocal_sameglobal": ("GaLc:GaLd", [False]), | ||
| "diff_local_and_global": ("GaLc:GbLd", [False]), |
There was a problem hiding this comment.
I had to work out what the single vs double result was about. Could do with a comment explaining this is for the op_leniency parameter.
Some for combine and for difference.
There was a problem hiding this comment.
I have completely now reviewed this, and am going to change it.
There was a problem hiding this comment.
Still looking into this.
See comment below about plans to reorganise the "matrix" testing
| "diffglobal_nolocal": ("GaL-:GbL-", [False]), | ||
| "diffglobal_samelocal": ("GaLc:GbLc", [False]), | ||
| "difflocal_noglobal": ("G-La:G-Lb", [False]), | ||
| "difflocal_sameglobal": ("GaLc:GaLd", [False]), |
There was a problem hiding this comment.
How did you choose which circumstances to cover?
For instance there is nothing for same-global, one-local (with one local being absent).
I can't tell if these are oversights, or deliberate decisions based on the mechanics.
There was a problem hiding this comment.
You're right, but it's triggered me to review all of this.
I'm now confident that more cases are needed, but also there are more factors I haven't accounted for.
Having prototyped a more comprehensive approach to that, I came up with a full matrix with 1152 tests (!!)
So I realised that I need to provide AFAP specific tests for each of the expected 'regularities' in behaviour, with more limited combinations of 'other' factors. The current plan for these :
- local and global values (for same attribute) should be calculated independently of each other
- calculations on a given attribute(name) should be calculated exactly the same way for both local and global settings
- each result for "left OP right" should be identical to "right OP left"
However, the basic calculation (combining a right + left value), must still be tested for each of the 3 operations, both strict+lenient, so there's still a fair number of tests.
I think I'm nearly on top of this now,
but I'm building a new testing mechanism, so don't fret about that code for now.
There was a problem hiding this comment.
I have now removed the idea of "targetted" testcases with selected combinations of factors.
The new approach is , broadly : more matrixing, leverage the expected 'same' results over different factors, and test the "local+global independence" separately from the other factors, to keep the number of tests under control.
Additional note:Spotted an outstanding bug here in the tests It seems clear that the second assignment to
|
An update@pp-mo and I realised that containing the specialised split-dict behaviour in method overrides in |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## FEATURE_split_attrs #5538 +/- ##
=======================================================
+ Coverage 89.25% 89.45% +0.20%
=======================================================
Files 88 90 +2
Lines 22197 22603 +406
Branches 4858 5437 +579
=======================================================
+ Hits 19811 20220 +409
+ Misses 1641 1638 -3
Partials 745 745 ☔ View full report in Codecov by Sentry. |
|
OK @trexfeathers I have just re-scanned + tweaked the comments+docstrings, and I have now definitely stopped fiddling with this. |
pp-mo
force-pushed
the
splitattrs_ncsave_redo_commonmeta
branch
from
dae3a28 to
b00b37f
Compare
trexfeathers
merged commit 0c20608
into
SciTools:FEATURE_split_attrs
* Split-attrs: Cube metadata refactortests (#4993) * Convert Test___eq__ to pytest. * Convert Test_combine to pytest. * Convert Test_difference to pytest. * Review changes. * Split attrs - tests for status quo (#4960) * Tests for attribute handling in netcdf load/save. * Tidy test functions. * Fix import order exception. * Add cf-global attributes test. * Towards more pytest-y implemenation. * Replace 'create_testcase' with fixture which also handles temporary directory. * Much tidy; use fixtures to parametrise over multiple attributes. * Fix warnings; begin data-style attrs tests. * Tests for data-style attributes. * Simplify setup fixture + improve docstring. * No parallel test runner, to avoid error for Python>3.8. * Fixed for new-style netcdf module. * Small review changes. * Rename attributes set 'data-style' as 'local-style'. * Simplify use of fixtures; clarify docstrings/comments and improve argument names. * Clarify testing sections for different attribute 'styles'. * Re-enable parallel testing. * Sorted params to avoid parallel testing bug - pytest#432. * Rename test functions to make alpha-order match order in class. * Split netcdf load/save attribute testing into separate sourcefile. * Add tests for loaded cube attributes; refactor to share code between Load and Roundtrip tests. * Add tests for attribute saving. * Fix method names in comments. * Clarify source of Conventions attributes. * Explain the test numbering in TestRoundtrip/TestLoad. * Remove obsolete test helper method. * Fix small typo; Fix numbering of testcases in TestSave. * Implement split cube attributes. (#5040) * Implement split cube attributes. * Test fixes. * Modify examples for simpler metadata printouts. * Added tests, small behaviour fixes. * Simplify copy. * Fix doctests. * Skip doctests with non-replicable outputs (from use of sets). * Tidy test comments, and add extra test. * Tiny typo. * Remove redundant redefinition of Cube.attributes. * Add CubeAttrsDict in module __all__ + improve docs coverage. * Review changes - small test changes. * More review changes. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix CubeAttrsDict example docstrings. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Odd small fixes. * Improved docstrings and comments; fix doctests. * Don't sidestep netcdf4 thread-safety. * Publicise LimitedAttributeDict, so CubeAttrsDict can refer to it. * Fix various internal + external links. * Update lib/iris/cube.py Co-authored-by: Martin Yeo <[email protected]> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <[email protected]> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <[email protected]> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <[email protected]> * Streamline docs. * Review changes. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Martin Yeo <[email protected]> * Splitattrs ncload (#5384) * Distinguish local+global attributes in netcdf loads. * Small test fixes. * Small doctest fix. * Fix attribute load-save tests for new behaviour, and old-behaviour equivalence. * Split attrs docs (#5418) * Clarification in CubeAttrsDict examples. * CubeAttrsDict fix docstring typo. * Raise awareness of split attributes in user guide. * What's New entry. * Changes to metadata documentation. * Splitattrs ncsave redo (#5410) * Add docs and future switch, no function yet. * Typing enables code completion for Cube.attributes. * Make roundtrip checking more precise + improve some tests accordingly (cf. #5403). * Rework all tests to use common setup + results-checking code. * Saver supports split-attributes saving (no tests yet). * Tiny docs fix. * Explain test routines better. * Fix init of FUTURE object. * Remove spurious re-test of FUTURE.save_split_attrs. * Don't create Cube attrs of 'None' (n.b. but no effect as currently used). * Remove/repair refs to obsolete routines. * Check all warnings from save operations. * Remove TestSave test numbers. * More save cases: no match with missing, and different cube attribute types. * Run save/roundtrip tests both with+without split saves. * Fix. * Review changes. * Fix changed warning messages. * Move warnings checking from 'run' to 'check' phase. * Simplify and improve warnings checking code. * Fix wrong testcase. * Minor review changes. * Fix reverted code. * Use sets to simplify demoted-attributes code. * WIP * Working with iris 3.6.1, no errors TestSave or TestRoundtrip. * Interim save (incomplete?). * Different results form for split tests; working for roundtrip. * Check that all param lists are sorted. * Check matrix result-files compatibility; add test_save_matrix. * test_load_matrix added; two types of load result. * Finalise special-case attributes. * Small docs tweaks. * Add some more testcases, * Ensure valid sort-order for globals of possibly different types. * Initialise matrix results with legacy values from v3.6.1 -- all matching. * Add full current matrix results, i.e. snapshot current behaviours. * Review changes : rename some matrix testcases, for clarity. * Splitattrs ncsave redo commonmeta (#5538) * Define common-metadata operartions on split attribute dictionaries. * Tests for split-attributes handling in CubeMetadata operations. * Small tidy and clarify. * Common metadata ops support mixed split/unsplit attribute dicts. * Clarify with better naming, comments, docstrings. * Remove split-attrs handling to own sourcefile, and implement as a decorator. * Remove redundant tests duplicated by matrix testcases. * Newstyle split-attrs matrix testing, with fewer testcases. * Small improvements to comments + docstrings. * Fix logic for equals expectation; expand primary/secondary independence test. * Clarify result testing in metadata operations decorator. * Splitattrs equalise (#5586) * Add tests in advance for split-attributes handling cases. * Move dict conversion inside utility, for use elsewhere. * Add support for split-attributes to equalise_attributes. * Update lib/iris/util.py Co-authored-by: Martin Yeo <[email protected]> * Update lib/iris/tests/unit/util/test_equalise_attributes.py Co-authored-by: Martin Yeo <[email protected]> * Simplify and clarify equalise_attributes code. --------- Co-authored-by: Martin Yeo <[email protected]> * Fix merge-fail messaging for attribute mismatches. (#5590) * Extra CubeAttrsDict methods to emulate dictionary behaviours. (#5592) * Extra CubeAttrsDict methods to emulate dictionary behaviours. * Don't use staticmethod on fixture. * Add Iris warning categories to saver warnings. * Type equality fixes for new flake8. * Licence header fixes. * Splitattrs ncsave deprecation (#5595) * Small improvement to split-attrs whatsnew. * Emit deprecation warning when saving without split-attrs enabled. * Stop legacy-split-attribute warnings from upsetting delayed-saving tests. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Martin Yeo <[email protected]>
Closes #5444
Specific handling of common-metadata operations on 'split' attribute dictionaries
(i.e. for
CubeMetadata)Consult Iris pull request check list